Fix race condition when flagging site as unsupported for application passwords #16237
Conversation
|
|
| guard let self else { return nil } | ||
|
|
||
| let retriedRequestIndex = _retriedJetpackRequests.firstIndex { retriedRequest in | ||
| let urlRequest = try? originalRequest.asURLRequest() |
There was a problem hiding this comment.
Small notice, might be negligible as _retriedJetpackRequests could be a few items, but we could move parsing originalRequest outside firstIndex so it only runs once.
There was a problem hiding this comment.
Good point! I moved the parsing of originalRequest to outside of the firstIndex block in b63ea52.
| let retriedRequestIndex = _retriedJetpackRequests.firstIndex { retriedRequest in | ||
| let urlRequest = try? originalRequest.asURLRequest() | ||
| let retriedRequest = try? retriedRequest.request.asURLRequest() | ||
| return urlRequest == retriedRequest |
There was a problem hiding this comment.
Relying on == for URLRequest may compare unstable fields, like header order. This can cause misses where the request is present but not found. However, I'm not sure what the intentions are here.
There was a problem hiding this comment.
Also, just want to point out, that if both try? are nil, we match the request to the original. Is this intended?
There was a problem hiding this comment.
I agree with both of your points - updated the implementation in b63ea52.
| } | ||
|
|
||
| // Force threads to start as close together as possible | ||
| group.wait() |
There was a problem hiding this comment.
Small note — the group.wait() here doesn’t actually make the threads start together; it just waits for them to finish. In this case, that’s probably fine since the goal is to ensure no crash under load, but I just wanted to flag it in case a true concurrent start was intended.
There was a problem hiding this comment.
I updated the comments in ed4f8fa, sorry for the misleading notes.
adborbas
left a comment
adborbas
left a comment
There was a problem hiding this comment.
I had some small comments, but otherwise, good job! I've tested as described.
|
@adborbas Thanks for the great reviews! I added subsequents commits to address the issues. Please feel free to take another look - I'll merge tomorrow. |
Closes WOOMOB-1450
Description
There are crash reports regarding the removal of retried requests when flagging sites as unsupported for application passwords. These are caused by the race condition when the list of retried requests is altered simultaneously from different threads.
This PR fixes the crash by ensuring that the retried request list is updated atomically with a barrier block.
Testing steps
It's not straightforward to reproduce race conditions so it's tricky to test the crash. Simply smoke testing the flow should be sufficient.
Use the test plan in pe5sF9-4Am-p2 - run TC4. Confirm that the app doesn't crash.
Testing information
Screenshots
N/A
RELEASE-NOTES.txtif necessary.